Skip to content

[CORE] Fix GlutenConfig runtime modifiability#12036

Open
malinjawi wants to merge 7 commits into
apache:mainfrom
malinjawi:fix/11998-gluten-config-modifiable
Open

[CORE] Fix GlutenConfig runtime modifiability#12036
malinjawi wants to merge 7 commits into
apache:mainfrom
malinjawi:fix/11998-gluten-config-modifiable

Conversation

@malinjawi
Copy link
Copy Markdown
Contributor

@malinjawi malinjawi commented May 4, 2026 •
edited
Loading

What changes are proposed in this pull request?

This PR fixes GlutenConfig runtime modifiability reporting for #11998.

GlutenConfig.get previously constructed GlutenConfig from SQLConf.get, the JVM thread-local SQLConf. Spark treats configs stored only in that static/thread-local SQLConf path as non-modifiable, so spark.conf.isModifiable("spark.gluten.*") could report false even for Gluten configs that are not static.

Main changes:

  • register Gluten config entries with Spark SQLConf metadata
  • preserve static Gluten config behavior by registering static configs through SQLConf.buildStaticConf
  • register config alternatives as SQLConf entries as well
  • update GlutenCoreConfig.get and GlutenConfig.get to read the active SparkSession SQLConf before falling back to SQLConf.get
  • add focused runtime config coverage for the exact SparkSession API path: spark.conf.isModifiable(...)
  • add focused runtime config coverage proving GlutenConfig.get reads active SparkSession config changes

Fixes #11998.

How was this patch tested?

Focused runtime config suites:

  • ./dev/run-scala-test.sh -Pjava-17,spark-4.0,scala-2.13,backends-velox,hadoop-3.3,spark-ut -pl gluten-ut/spark40 -s org.apache.spark.sql.GlutenRuntimeConfigSuite
  • ./dev/run-scala-test.sh -Pjava-17,spark-4.1,scala-2.13,backends-velox,hadoop-3.3,spark-ut -pl gluten-ut/spark41 -s org.apache.spark.sql.GlutenRuntimeConfigSuite

Also checked:

  • git diff --check

Was this patch authored or co-authored using generative AI tooling?

Generated-by: IBM BOB

@github-actions github-actions Bot added the CORE works for Gluten Core label May 4, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

Run Gluten Clickhouse CI on x86

1 similar comment
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 4, 2026

Run Gluten Clickhouse CI on x86

@philo-he philo-he changed the title [VL] Fix GlutenConfig runtime modifiability [CORE] Fix GlutenConfig runtime modifiability May 4, 2026
@philo-he philo-he self-requested a review May 4, 2026 16:58
@FelixYBW
Copy link
Copy Markdown
Contributor

FelixYBW commented May 4, 2026

@malinjawi thank you for the PR. Can you add one column to https://github.com/apache/gluten/blob/main/docs/velox-configuration.md and mark each config as static (⚓) or dynamic (🔄)?

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Run Gluten Clickhouse CI on x86

@malinjawi malinjawi force-pushed the fix/11998-gluten-config-modifiable branch from 116ad3c to b6c19f6 Compare May 5, 2026 13:19
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

Run Gluten Clickhouse CI on x86

@FelixYBW
Copy link
Copy Markdown
Contributor

FelixYBW commented May 5, 2026

thank you, @malinjawi. Can you fix Configuration.md as well? our config file is automatically created. Can you change the script instead?

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 7, 2026

Run Gluten Clickhouse CI on x86

@malinjawi
Copy link
Copy Markdown
Contributor Author

malinjawi commented May 7, 2026 •
edited
Loading

Thank you @FelixYBW! I updated and regenerated the docs through dev/gen-all-config-docs.sh

Verified with:

  • dev/gen-all-config-docs.sh
  • GLUTEN_UPDATE=0 ./build/mvn test -Pbackends-velox -pl backends-velox -am -Dtest=none -DfailIfNoTests=false -DwildcardSuites=org.apache.gluten.config.AllGlutenConfiguration,org.apache.gluten.config.AllVeloxConfiguration
  • ./dev/format-scala-code.sh check
  • git diff --check

@malinjawi malinjawi force-pushed the fix/11998-gluten-config-modifiable branch from 7054c17 to 0703881 Compare May 13, 2026 10:56
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

Run Gluten Clickhouse CI on x86

philo-he
philo-he reviewed May 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@philo-he philo-he left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments. Thanks.

Comment thread gluten-ut/spark41/src/test/scala/org/apache/spark/sql/execution/GlutenSparkSqlParserSuite.scala Outdated

override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
pos: Position): Unit = {
if (isConfigParserCoverage(testName) && !registerQuotedConfigParserTest) {
Copy link
Copy Markdown
Member

@philo-he philo-he May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the original Spark test is excluded and replaced by the variant test? If so, can we just exclude it in VeloxTestSettings.scala and let the variant be tested instead?

testGluten("Checks if SET/RESET can parse all the configurations") {

}

Copy link
Copy Markdown
Contributor Author

@malinjawi malinjawi May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @philo-he, addressed. I removed the custom test(...) override, excluded Spark's original Checks if SET/RESET can parse all the configurations case in VeloxTestSettings.scala, and registered the quoted-key coverage as a normal testGluten(...) variant.

Comment thread gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenRuntimeConfigSuite.scala Outdated
assert(!conf.isModifiable(GlutenConfig.GLUTEN_UI_ENABLED.key))
}

test("GlutenConfig reads active SparkSession runtime configs") {
Copy link
Copy Markdown
Member

@philo-he philo-he May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move these two tests to a new test suite under gluten-ut/test? Generally, the spark{VERSION} folders are used for Spark's original tests and their version-specific variants.

Copy link
Copy Markdown
Contributor Author

@malinjawi malinjawi May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @philo-he, moved these two Gluten-specific tests into a new shared suite under gluten-ut/test: org.apache.gluten.config.GlutenRuntimeConfigSuite.

I also restored the Spark 4.0 / 4.1 GlutenRuntimeConfigSuite files to only wrap Spark's original RuntimeConfigSuite.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 14, 2026

Run Gluten Clickhouse CI on x86

@malinjawi malinjawi requested a review from philo-he May 14, 2026 12:26
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 14, 2026

Run Gluten Clickhouse CI on x86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FelixYBW FelixYBW FelixYBW approved these changes

@philo-he philo-he Awaiting requested review from philo-he

Assignees

No one assigned

Labels

CORE works for Gluten Core DOCS VELOX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[VL] Refactor GlutenConfig to reflect the right status under isModifiable()

3 participants

@malinjawi @FelixYBW @philo-he